Skip to content

gh-140544: fix build for including pycore_pystate.h when HAVE_THREAD_LOCAL is not defined #140623

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Oct 27, 2025
Merged

gh-140544: fix build for including pycore_pystate.h when HAVE_THREAD_LOCAL is not defined #140623

merged 2 commits into from
Oct 27, 2025
+2 −2

Conversation

@kumaraditya303
Copy link
Contributor

@kumaraditya303 kumaraditya303 commented Oct 26, 2025 •
edited by bedevere-app bot
Loading

@bedevere-app bedevere-app bot mentioned this pull request Oct 26, 2025
Closed
@kumaraditya303 kumaraditya303 mentioned this pull request Oct 26, 2025
Merged
@kumaraditya303
Copy link
Contributor Author

cc @cdce8p

cdce8p
cdce8p approved these changes Oct 26, 2025
View reviewed changes
Copy link
Contributor

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes the issue. Thanks @kumaraditya303!

@kumaraditya303 kumaraditya303 marked this pull request as ready for review October 26, 2025 14:05
ZeroIntensity
ZeroIntensity reviewed Oct 26, 2025
View reviewed changes
ZeroIntensity
ZeroIntensity approved these changes Oct 27, 2025
View reviewed changes
Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks.

@kumaraditya303 kumaraditya303 merged commit f5394c2 into python:main Oct 27, 2025
47 checks passed
@kumaraditya303 kumaraditya303 deleted the tls branch October 27, 2025 20:10
@cdce8p
Copy link
Contributor

cdce8p commented Oct 27, 2025

Unfortunately it seems the last change broke it again.

/.../cpython/Include/internal/pycore_pystate.h:93:8: error: unknown type name '_Py_thread_local'
extern _Py_thread_local PyThreadState *_Py_tss_tstate;

/.../cpython/Include/internal/pycore_pystate.h:93:38: error: expected ';' after top level declarator
extern _Py_thread_local PyThreadState *_Py_tss_tstate;
                                     ^
                                     ;

It looks like _Py_thread_local only works for defined(HAVE_THREAD_LOCAL). So 34a2688 should probably be reverted.

cpython/Include/pyport.h

Lines 516 to 528 in f5394c2

# define HAVE_THREAD_LOCAL 1
# ifdef thread_local
# define _Py_thread_local thread_local
# elif __STDC_VERSION__ >= 201112L && !defined(__STDC_NO_THREADS__)
# define _Py_thread_local _Thread_local
# elif defined(_MSC_VER) /* AKA NT_THREADS */
# define _Py_thread_local __declspec(thread)
# elif defined(__GNUC__) /* includes clang */
# define _Py_thread_local __thread
# else
// fall back to the PyThread_tss_*() API, or ignore.
# undef HAVE_THREAD_LOCAL
# endif

@ZeroIntensity
Copy link
Member

No, this is all just HAVE_THREAD_LOCAL being messy. We should remove that all throughout the codebase and assume that thread locals are always available. mypyc will get some speedups from using the thread local directly. I'll put up a PR.

@cdce8p
Copy link
Contributor

cdce8p commented Oct 27, 2025

Sounds good. Happy to test it, just let me know.

@cdce8p cdce8p mentioned this pull request Oct 27, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ZeroIntensity ZeroIntensity ZeroIntensity approved these changes

@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner

+1 more reviewer

@cdce8p cdce8p cdce8p approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kumaraditya303 @cdce8p @ZeroIntensity